Add the abilty to resend code though bluetooth in the same session #122
Conversation
|
I haven't used poetry much, so I wasn't aware of the versioning conflicts. I have confirmed that the lock file has now been built correctly. |
dlech
left a comment
dlech
left a comment
There was a problem hiding this comment.
Thanks for having a go a this. This has been an often requested feature that we just haven't had time to prioritize.
What you suggest is fine for a bare minimum solution. If we want to go a bit fancier though, we could make an interactive command prompt mode that basically inverts what you have done here. I.e. start with the prompt/menu and use that to connect, download and run, disconnect, etc.
|
The prompt idea sounds fun to implement, I'll take a look at doing that. |
|
I went ahead and implemented all the suggestions you had. I didn't directly use prompt_toolkit, but I found an async compatible library that is built on prompt_toolkit for arrow-key selections (more user friendly IMO). Turning off the robot and trying to resend raises a runtime error, but I took care of that with an except block. Turning off the robot and then selecting exit raises no errors. I think that making a full blown TUI would be a lot more invasive, maybe it could be the subject of a separate PR? I also feel like it would be a good idea to keep all of the manual commands with arguments available to users and maybe only bring up a TUI if the program is executed |
|
All workflows should pass now. I had to add the stay_connected arg to all of the unit tests involving downloading to or running code on the robot. It is currently set to false on all of the tests. |
|
I did find one interesting edge case. If you use the wait arg and the say_connected arg but not the start arg, the program will be downloaded to to the hub without running and it will stay connected and allow resending. While this behavior makes sense, we may want to modify the help text of the wait arg to signify that it now does have an effect if the start arg is false. |
dlech
left a comment
dlech
left a comment
There was a problem hiding this comment.
While this behavior makes sense, we may want to modify the help text of the wait arg to signify that it now does have an effect if the start arg is false.
I would think we would want make "stay connected" always imply "wait". For the CLI this would mean the user only has to type one argument. And for the Python API, we could consider to make wait a 3-state option rather than adding a new parameter. I.e. options are "don't wait and return immediately", "wait for program to end and then return", "wait for program to end and then show menu"
pybricksdev/cli/__init__.py
Outdated
| if not args.wait or not args.stay_connected: | ||
| break | ||
|
|
||
| resend = await questionary.select( |
There was a problem hiding this comment.
I think we could use race_disconnect() here to cancel the menu if the hub becomes disconnected rather than waiting for the user to respond.
There was a problem hiding this comment.
Yes, that would be a much cleaner option. Since we would be properly handling the disconnect event, maybe it would be a good idea to prompt the user to re-connect?
There was a problem hiding this comment.
That sounds like it would make the implementation quite a bit more complex. So I would say no. Or at least save that for implementing later.
After the program exits because of disconnect, the user should be able to just press up to get the command from the history in their terminal and run it again to reconnect.
pybricksdev/cli/__init__.py
Outdated
| if resend == "Exit": | ||
| break | ||
|
|
||
| except RuntimeError: |
There was a problem hiding this comment.
Is disconnection really the only possible thing that could happen to cause a RuntimeError?
pybricksdev/cli/__init__.py
Outdated
| break | ||
|
|
||
| resend = await questionary.select( | ||
| "Would you like to resend your code?", choices=["Resend", "Exit"] |
There was a problem hiding this comment.
"Resend" sounds like it would resend exactly the same program again. But in that case, the program is already on the hub, so there is no reason to send it again. We can just send a command to start the program again without taking the time to send it.
But if the user locally modified their source code and we need to compile and download again, that is a different case. So perhaps we should have 3 menu items?
There was a problem hiding this comment.
Regarding the case of compiling and downloading a changed program, we would want to catch the error if compiling fails (e.g. a syntax error) so that we don't disconnect in that case.
There was a problem hiding this comment.
With the current setup, it will automatically recompile whatever is at the file path. I think it makes sense to have that as the only option, but the text choice could be something more like "Recompile and Run". Maybe there could be a separate option for "Recompile and Download?
There was a problem hiding this comment.
I suppose. As long as the default matches the original command line option for --start/--no-start.
pybricksdev/cli/__init__.py
Outdated
| "Would you like to resend your code?", choices=["Resend", "Exit"] | ||
| ).ask_async() | ||
|
|
||
| if resend == "Exit": |
There was a problem hiding this comment.
| if resend == "Exit": | |
| if response == "Exit": |
Would make more sense to me if the variable name wasn't one of the options.
|
I just came across an issue. When you run the program from the hub, it will mess up the prompt if it prints out a lot. If you press the down arrow, you can still see the options, but they aren't rendered properly and the question is gone. I couldn't find a reliable way to check if the hub is running a program, so I'm thinking that the best option would probably be to lock stdout while the prompt is up and add a prompt option to refresh the console. |
|
I was expecting that we would always wait for the program to finish before showing the prompt so that this sort of problem could not happen. |
Yes, that is correct. However, you can manually re-run the current program with the hub's center button, and the hub's output is piped to the computer's stdout as long as it is still connected through bluetooth. I don't know of a reliable way to check when a program is manually run from the hub, but I could be missing something. |
|
Ah, I didn't consider that case. The hub sends a status message with a flag set to indicate that a program is running. So we should be able to detect that and make something similar to the race disconnect method to cancel the menu if that happens. |
|
Ok, sweet! What would be the easiest way to detect that flag? And is there another flag that gets sent to signify the end of the program? |
|
The same flag would be cleared when the program ends. Have a look at |
I totally agree, and that is the first thing I tried. Unfortunately, the output it mangled if the hub sends to stdout immediately after the program starts: Whereas the power button press is much nicer: To be clear, nothing errors out and the program continues as if nothing happened, but I would prefer to avoid mangling the users' print statements if possible. |
|
I added the custom errors, but they are currently only triggered by the race functions. I also forgot to test if I handle them properly, turns out I don't. One moment. |
|
Ok, that should be fixed now. |
|
Are the names suitable, or should I change them? They feel a tad long to me, but I'm not sure if that's really an issue. |
dlech
left a comment
dlech
left a comment
There was a problem hiding this comment.
I think this is looking pretty good. Just have some suggestions for polishing it up a bit now.
pybricksdev/cli/__init__.py
Outdated
|
|
||
| except HubPowerButtonPressedError: | ||
| try: | ||
| await hub._wait_for_user_program_stop(5) |
There was a problem hiding this comment.
Why do we need a different timeout here? 5 seconds seems a bit long.
There was a problem hiding this comment.
The time really doesn't matter as long as it is higher than the time it takes for the power button press to trigger a shutdown. I was just playing it safe in case a different firmware version takes longer to do that. The user doesn't end up waiting any longer than they would if it were 2.1 seconds.
I think they are fine. |
Sweet! Do you see anything else that needs fixing? |
|
Besides the 14 comments I just made? 😁 |
|
It looks like the tests didn't like me calling exit if args.stay_connected is false. I changed it to just return instead in spots where it makes sense. I feel like using exit within the reconnect_hub function is still probably the best move. |
Wow, really thought I checked that... Obviously not. I think that I got about all of it, let me know if I need to do anything else. |
dlech
left a comment
dlech
left a comment
There was a problem hiding this comment.
I think it looks good now, but I did some testing and found a few places where things weren't working as expected. See inline comments for suggested fixes.
It also exposed an existing bug in race_disconnect() that I fixed in bb9b718, so you can rebase or merge to pull in that fix.
Ahh, that explains why I couldn't nest race-disconnect inside of another race function! I'll add this to the other race function as well. Do you think it would be a good idea to remove the disconnect event from the other race function and nest it when called now that this is fixed? |
I don't have a strong preference either way. |
…ction doesn't detect the hub being turned off properly
…file is sys.stdin
|
The only CI issue seems to be that coveralls had a bad gateway, which (I think?) should be server side. I am a tad confused on why that happened specifically on 3.10 twice in a row. |
Yeah, this seems to happen quite frequently with coveralls, not just on this project. So nothing to worry about. |
|
I changed the behavior to use the button being released instead of a timeout. It seems to work pretty well in my testing, but let me know if I'm missing anything. |
dlech
left a comment
dlech
left a comment
There was a problem hiding this comment.
I just tested again and it seems to be working well. Thanks a lot for the work on this!
2 participants
One of the primary issues that pushed me to use the web IDE over the pybricksdev cli tool is that it automatically disconnects from bluetooth as soon as the program finishes. This means that the computer has to re-connect to the hub over bluetooth each time you want to send your program. While this doesn't take terribly long, it adds up over time.
My solution is to place the compiling and flashing functions inside of a while loop and show a menu asking if the user wants to re-send their code or exit. This behavior is disabled by default and can be enabled with the --resend argument. It also automatically defaults back to standard behavior if the --wait arg is false or the connection type is usb.
I have tested this with a pybricks hub running the latest beta software, and I can't find any immediate issues with it, except that a BleakError is raised when trying to resend code if the robot has been powered off.